scrotGrabMousePointer: error out on failure #317
Conversation
initially this path wasn't chosen because we suspected this might violate C's "strict-aliasing" rule. but a more careful look shows that this is not the case. xcim->pixels (due to being dynamically allocated memory) does not have any declared type, and so it's effective type is determined by the last write, in our case `unsigned long`. pixels[i] = xcim->pixels[i]; in all iteration of the loop, `xcim->pixels[i]` at the time of the read, has the effective type of `unsigned long` which is what we are reading it as, so there's no issue. the `pixels[i]` write changes the written-to region's effective type to `uint32_t` but that value is never read until the `imlib_create_image_using_data` call later, which reads it as `uint32_t` - matching it's effective type. as such, there's no real strict-aliasing violation here and we can simply fixup the array in place. this reduces a potential runtime error and (in theory) uses less memory. ref: https://port70.net/~nsz/c/c99/n1256.html#6.5p6 ref: resurrecting-open-source-projects#193 (comment)
if the user asks us to grab the pointer, we should either do that otherwise fail rather than continuing on. also remove a redundant comment, it should be clear that a function named XFixesGetCursorImage is indeed returning back a cursor image. ref: resurrecting-open-source-projects#272
|
Let's try not to place commits that do not go with what is indicated by the PR (I understand, I also did it sometimes) |
| uint32_t *pixels = (uint32_t *)xcim->pixels; | ||
| /* XFixesCursorImage returns pixels as `unsigned long`, which is typically | ||
| * 64bits, but imlib2 expects 32bit packed integers. */ | ||
| for (size_t i = 0; i < pixcnt; i++) | ||
| pixels[i] = xcim->pixels[i]; | ||
| if (sizeof *xcim->pixels > sizeof *pixels) { | ||
| for (size_t i = 0; i < pixcnt; ++i) | ||
| pixels[i] = xcim->pixels[i]; | ||
| } |
There was a problem hiding this comment.
This does violate C's aliasing rules: https://www.iso-9899.info/n1570.html#6.5p7
There was a problem hiding this comment.
Yes, I've read that and explained in the commit why there's no violation. If you think otherwise then feel free to explain in more detail.
There was a problem hiding this comment.
To elaborate, each element of an array is an object by itself (C99 § 3.14). The write into u32[i] only changes the effective type of that region, not the entire array, the rest of the elements still retain their effective type because they haven't been written to, C99 § 6.5:
If a value is stored into an object having no declared type through an lvalue having a type that is not a character type, then the type of the lvalue becomes the effective type of the object for that access and for subsequent accesses that do not modify the stored value.
There was a problem hiding this comment.
It's perfectly valid as far as C goes that xcim->pixels is aligned to unsigned long, but not uint32_t. There is no requirement that uint32_t's alignment requirements have to be <= unsigned long's, or that unsigned long's alignment requirements have to be an integer multiple of uint32_t's.
Perhaps we can let it slip, Eric S. Raymond's struct packing guide says:
I’ve learned something rather reassuring from working with the source code for the reference implementation of NTP. It does packet analysis by reading packets off the wire directly into memory that the rest of the code sees as a struct, relying on the assumption of minimal self-aligned padding - or zero padding in odd cases like 690x0.
The interesting news is that NTP has apparently being getting away with this for decades across a very wide span of hardware, operating systems, and compilers, including not just Unixes but under Windows variants as well. This suggests that platforms with padding rules other than self-alignment are either nonexistent or confined to such specialized niches that they’re never either NTP servers or clients.
i.e. in all modern machines _Alignof(type) == sizeof(type), although I know that long double can be an exception on i386 with its 80-bit FPU: it has sizeof(long double) == 10 and _Alignof(long double) == 16 in the SysV ABI.
There was a problem hiding this comment.
Also, no unnecessary casts as in line 458 please.
There was a problem hiding this comment.
Also, no unnecessary casts as in line 458 please.
The cast wasn't there on my first iteration, but it caused a compiler warning. So I added it.
There is no requirement that
uint32_t's alignment requirements have to be <=unsigned long's, or thatunsigned long's alignment requirements have to be an integer multiple ofuint32_t's.
Okay, that's fair. I was only thinking of aliasing and didn't consider alignment. But if such a machine exists, I kinda doubt Imlib2 will work there, because IIRC I saw some of the Imlib2 loaders treat raw memory as struct as well - which would most likely be broken under such machines.
There was a problem hiding this comment.
it caused a compiler warning
Ugh, compilers forcing us to write C++-like C.
Edit: actually, I guess it's not the compiler's fault if we write known UB and it warns, the unnecessary cast is the "I know what I'm doing" idiom of C.
This turns the previous warnings into errors instead - since we couldn't accomplish what the user asked us to.
And while working on this, I also realized that the in-place mutation wouldn't violate strict aliasing and is legal. So remove the failure point created by
mallocentirely.ref: #272